Fix deadlock releasing TSFN after environment shutdown - #480
Fix deadlock releasing TSFN after environment shutdown#480tetra (tetra-fox) wants to merge 1 commit into
Conversation
|
Would be great to get this merged! |
|
/azp run |
|
No pipelines are associated with this pull request. |
|
It is strange that this PR did not run the usual set of PR validations. |
There was a problem hiding this comment.
Pull request overview
Fixes a Node.js >= 24.14.0 shutdown-time deadlock by avoiding napi_release_threadsafe_function on a TSFN that Node has already finalized, preventing re-locking the TSFN mutex during environment teardown.
Changes:
- Track whether the TSFN has been finalized via a TSFN finalize callback.
- Skip
_tsfn.Release()duringJSTsfnSynchronizationContext.Dispose()once finalization has occurred.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/NodeApi/Interop/JSSynchronizationContext.cs:277
- Read _tsfnFinalized using Volatile.Read to pair with the finalizer write and ensure Dispose() observes the finalized state correctly if called across threads.
if (!_tsfnFinalized)
src/NodeApi/Interop/JSSynchronizationContext.cs:258
- The TSFN finalizer writes _tsfnFinalized from an unmanaged callback; Dispose() may be invoked from a different thread (JSThreadSafeFunction.Release is documented here as callable from any thread). Use Volatile.Write to avoid a data race and to make the cross-thread intent explicit.
asyncResourceName: (JSValue)nameof(JSSynchronizationContext),
finalize: _ => _tsfnFinalized = true);
|
tetra (@tetra-fox) , thank you for your contribution and for tracking down this issue. Unfortunately, I wasn't able to merge this PR directly because of an issue with GitHub that prevented the repository's validation workflow from running on the PR. Since I couldn't find a way to re-trigger the validation, the PR remained open longer than expected. While we were investigating that, another team needed a fix urgently and landed a slightly modified version in PR #486. I want to make sure you receive credit for the work: your investigation identified the problem, and your proposed fix helped inform the solution that was ultimately merged. Since the issue has now been resolved through PR #486, I'm closing this PR. Thank you again for your contribution, especially as a first-time contributor. We truly appreciate the time and effort you invested, and I hope you'll consider contributing again in the future. |
|
The node-api-dotnet version that contains the fix is 0.9.23. |
Since nodejs/node#55877 (appeared in Node 24.14.0), environment shutdown finalizes a TSFN that still has use counts by releasing its resources while holding the TSFN mutex. Dropping the env reference there runs napi finalizers synchronously. The instance data finalizer disposes
JSRuntimeContext, whoseJSTsfnSynchronizationContext.Dispose()callednapi_release_threadsafe_functionon the same TSFN and re-locked a mutex the thread already holds. The JS thread deadlocks against itself. On older Node the stale release was a silent use-after-free. In VRCX (Electron 40 / .NET 9 / Linux) every quit hung the process:thread_finalize_cbfires when Node.js destroys the TSFN, before the instance data finalizer runs. The fix tracks it and skips the release once it has fired. The normal dispose path is unchanged, and both the callback andDispose()run on the JS thread, so there is no race on the flag. The finalization docs describe this ordering and warn about use-after-free innapi_finalizecallbacks.Repro on Node.js >= 24.14.0 (Linux x64, stock
mcr.microsoft.com/dotnet/runtime:9.0container):npm install node-api-dotnet@0.9.21, thennode -e "require('node-api-dotnet/net9.0')"never exits, parked on the stack above. With this change it exits immediately. On Node.js <= 24.13.0 the one-liner exits normally. Quit in VRCX hung 3/3 times on stock and exits cleanly 3/3 on this patch, and the existing suite passes.Considered but left out: the same guard on the other TSFN entry points (
Ref/Unref,Post/Send), and disposingJSRuntimeContextfrom a cleanup hook registered after the TSFN is created, so it runs while the TSFN is still alive (the pattern those docs recommend). The latter changes disposal timing during shutdown, so it seemed like a separate discussion.